Conversation
…to add-logging-stack-v2
…to add-logging-stack-v2
There was a problem hiding this comment.
Pull request overview
This PR introduces a new pkg/observability layer (logger + metrics adapters) and wires it into the GitHub tool dependency construction for both HTTP and stdio server paths.
Changes:
- Added
pkg/observabilitywith backend-agnosticlog.Logger/metrics.Metricsinterfaces plus noop +slogadapters and tests. - Extended
pkg/githubdependency plumbing to provideLogger()/Metrics()onToolDependencies, and to accept a*slog.LoggerinNewBaseDeps/NewRequestDeps. - Threaded the HTTP server’s
slog.Loggerinto request deps and the stdio server’scfg.Loggerinto base deps.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/observability/observability.go | Defines Exporters interface and basic implementation. |
| pkg/observability/observability_test.go | Unit tests for NewExporters. |
| pkg/observability/log/logger.go | Adds backend-agnostic logger interface. |
| pkg/observability/log/level.go | Adds log level type/constants. |
| pkg/observability/log/field.go | Adds structured field type + helpers. |
| pkg/observability/log/noop_adapter.go | No-op logger implementation. |
| pkg/observability/log/slog_adapter.go | slog-backed logger adapter. |
| pkg/observability/log/slog_adapter_test.go | Tests for SlogLogger. |
| pkg/observability/metrics/metrics.go | Adds backend-agnostic metrics interface. |
| pkg/observability/metrics/noop_adapter.go | No-op metrics implementation. |
| pkg/observability/metrics/noop_adapter_test.go | Tests for NoopMetrics. |
| pkg/observability/metrics/slog_adapter.go | slog-backed metrics adapter. |
| pkg/observability/metrics/slog_adapter_test.go | Tests for SlogMetrics. |
| pkg/github/dependencies.go | Adds observability to ToolDependencies; updates constructors to accept *slog.Logger. |
| pkg/github/dependencies_test.go | Updates constructor calls for new signature. |
| pkg/github/feature_flags_test.go | Updates constructor calls for new signature. |
| pkg/github/dynamic_tools_test.go | Updates constructor calls for new signature. |
| pkg/github/server_test.go | Updates test deps to provide Logger/Metrics. |
| pkg/http/server.go | Passes HTTP server logger into NewRequestDeps. |
| internal/ghmcp/server.go | Passes cfg.Logger into NewBaseDeps. |
Comments suppressed due to low confidence (3)
pkg/github/dependencies.go:145
- NewBaseDeps’ signature now requires a *slog.Logger, which is a breaking change for downstream callers. Consider preserving the existing constructor (old signature) and adding a new constructor/option for observability (e.g., NewBaseDepsWithLogger or functional options) to avoid forcing updates for all consumers.
flags FeatureFlags,
contentWindowSize int,
featureChecker inventory.FeatureFlagChecker,
logger *slog.Logger,
) *BaseDeps {
pkg/github/dependencies.go:296
- NewRequestDeps’ signature now requires a *slog.Logger, which is a breaking change for downstream callers. Consider keeping the old signature and adding an alternate constructor/option for supplying a logger to avoid a hard API break.
t translations.TranslationHelperFunc,
contentWindowSize int,
featureChecker inventory.FeatureFlagChecker,
logger *slog.Logger,
) *RequestDeps {
pkg/github/server_test.go:76
- stubDeps.Metrics ignores the provided ctx and calls the exporter with context.Background(), which can hide context-dependent metrics behavior in tests. Pass the incoming ctx through to s.obsv.Metrics(ctx) for consistency.
func (s stubDeps) Metrics(_ context.Context) mcpMetrics.Metrics {
if s.obsv != nil {
return s.obsv.Metrics(context.Background())
}
return mcpMetrics.NewNoopMetrics()
| // Logger returns the logger | ||
| Logger(ctx context.Context) obsvLog.Logger | ||
|
|
||
| // Metrics returns the metrics client | ||
| Metrics(ctx context.Context) obsvMetrics.Metrics |
There was a problem hiding this comment.
Adding Logger/Metrics to ToolDependencies is a breaking interface change for any external dependency provider (e.g., remote server or third-party users). If backward compatibility is required, consider introducing a separate optional interface (and type-assert at call sites) or a new ToolDependenciesV2, rather than extending the existing interface directly.
This issue also appears in the following locations of the same file:
- line 141
- line 292
| var obsv observability.Exporters | ||
| if logger != nil { | ||
| obsv = observability.NewExporters(obsvLog.NewSlogLogger(logger, obsvLog.InfoLevel), obsvMetrics.NewNoopMetrics()) | ||
| } else { | ||
| obsv = observability.NewExporters(obsvLog.NewNoopLogger(), obsvMetrics.NewNoopMetrics()) |
There was a problem hiding this comment.
The default observability/exporters initialization logic is duplicated in NewBaseDeps and NewRequestDeps. Consider extracting a small helper (e.g., newDefaultExporters(logger *slog.Logger) observability.Exporters) to keep behavior consistent and avoid future drift.
| func (s stubDeps) Logger(_ context.Context) mcpLog.Logger { | ||
| if s.obsv != nil { | ||
| return s.obsv.Logger(context.Background()) | ||
| } | ||
| return mcpLog.NewNoopLogger() | ||
| } | ||
| func (s stubDeps) Metrics(_ context.Context) mcpMetrics.Metrics { | ||
| if s.obsv != nil { | ||
| return s.obsv.Metrics(context.Background()) |
There was a problem hiding this comment.
stubDeps.Logger ignores the provided ctx and calls the exporter with context.Background(), which can mask context-dependent logging behavior in tests. Pass the incoming ctx through to s.obsv.Logger(ctx) to better reflect real execution.
This issue also appears on line 72 of the same file.
| func (s stubDeps) Logger(_ context.Context) mcpLog.Logger { | |
| if s.obsv != nil { | |
| return s.obsv.Logger(context.Background()) | |
| } | |
| return mcpLog.NewNoopLogger() | |
| } | |
| func (s stubDeps) Metrics(_ context.Context) mcpMetrics.Metrics { | |
| if s.obsv != nil { | |
| return s.obsv.Metrics(context.Background()) | |
| func (s stubDeps) Logger(ctx context.Context) mcpLog.Logger { | |
| if s.obsv != nil { | |
| return s.obsv.Logger(ctx) | |
| } | |
| return mcpLog.NewNoopLogger() | |
| } | |
| func (s stubDeps) Metrics(ctx context.Context) mcpMetrics.Metrics { | |
| if s.obsv != nil { | |
| return s.obsv.Metrics(ctx) |
1 participant
Summary
Why
Fixes #
What changed
MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs